PhotoLocatorTest/BenchmarkHelper.cs (1)

6-6: Two small polish items.

• BenchmarkHelper exposes only static members; mark the class static to prevent instantiation.
• sw.ElapsedMilliseconds truncates to whole milliseconds, which can hide differences for fast operations; sw.Elapsed.TotalMilliseconds (double) or sw.ElapsedTicks would be more useful for benchmarking. The iterationTimes array would need to become double[]/long[] accordingly.

♻️ Suggested polish

-    public class BenchmarkHelper
+    public static class BenchmarkHelper
     {
         public static void Run(Action action, int innerLoops = 1, int outerIterations = 5)
         {
             ...
-            var iterationTimes = new long[outerIterations];
+            var iterationTimes = new double[outerIterations];
             ...
-                Console.WriteLine($"Iteration {i + 1}: {sw.ElapsedMilliseconds} ms");
+                Console.WriteLine($"Iteration {i + 1}: {sw.Elapsed.TotalMilliseconds:F3} ms");
                 ...
-                iterationTimes[i] = sw.ElapsedMilliseconds;
+                iterationTimes[i] = sw.Elapsed.TotalMilliseconds;

Also applies to: 24-24

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@PhotoLocatorTest/BenchmarkHelper.cs` at line 6, BenchmarkHelper currently
exposes only static members and uses sw.ElapsedMilliseconds which truncates
timing precision; mark class BenchmarkHelper as static to prevent instantiation,
change the iterationTimes array to double[] (or long[] if you prefer ticks) and
replace uses of sw.ElapsedMilliseconds with sw.Elapsed.TotalMilliseconds (or
sw.ElapsedTicks) and update all related method signatures/returns that reference
iterationTimes or its element type (e.g., any methods that build/return
iterationTimes) so types align with the new double[]/long[] choice.

PhotoLocator/BitmapOperations/IIRMinMaxOperation.cs (1)

51-51: Nit: typo quaterWidth → quarterWidth.

The refactor logic is correct (each of the 4 column states is independent and the tail loop covers the Stride % 4 remainder), but the variable name is misspelled in both MinFilter and MaxFilter.

✏️ Suggested rename

-                    int quaterWidth = plane.Stride / 4;
-                    Parallel.For(0, quaterWidth, x4 =>
+                    int quarterWidth = plane.Stride / 4;
+                    Parallel.For(0, quarterWidth, x4 =>
                     ...
-                    Parallel.For(quaterWidth * 4, plane.Stride, x =>
+                    Parallel.For(quarterWidth * 4, plane.Stride, x =>

Also applies to: 103-103, 174-174, 226-226

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@PhotoLocator/BitmapOperations/IIRMinMaxOperation.cs` at line 51, The variable
name `quaterWidth` is misspelled in IIRMinMaxOperation.cs; rename it to
`quarterWidth` across the file so both MinFilter and MaxFilter use the correct
identifier (update the declaration and all usages in methods MinFilter and
MaxFilter and the other occurrences noted) to keep naming consistent and avoid
confusion or potential shadowing issues.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

PhotoLocatorTest/BenchmarkHelper.cs (1)

16-25: ⚠️ Potential issue | 🟠 Major

GC.EndNoGCRegion() still called unconditionally.

GC.TryStartNoGCRegion can return false (e.g., on memory-constrained machines or if a no‑GC region is already active), in which case the unconditional GC.EndNoGCRegion() on line 23 will throw InvalidOperationException and abort the benchmark loop. Capture the return value and only end the region when it actually started. A try/finally around the timed work would also ensure the region is closed if action() throws.

Proposed fix

             for (int i = 0; i < outerIterations; i++)
             {
                 GC.Collect();
-                GC.TryStartNoGCRegion(1024 * 1024 * 100);
-                var sw = Stopwatch.StartNew();
-                for (int j = 0; j < innerLoops; j++)
-                    action();
-                sw.Stop();
-                Console.WriteLine($"Iteration {i + 1}: {sw.ElapsedMilliseconds} ms");
-                GC.EndNoGCRegion();
-                iterationTimes[i] = sw.ElapsedMilliseconds;
+                bool inNoGc = GC.TryStartNoGCRegion(1024 * 1024 * 100);
+                var sw = Stopwatch.StartNew();
+                try
+                {
+                    for (int j = 0; j < innerLoops; j++)
+                        action();
+                }
+                finally
+                {
+                    sw.Stop();
+                    if (inNoGc && GCSettings.LatencyMode == GCLatencyMode.NoGCRegion)
+                        GC.EndNoGCRegion();
+                }
+                Console.WriteLine($"Iteration {i + 1}: {sw.ElapsedMilliseconds} ms");
+                iterationTimes[i] = sw.ElapsedMilliseconds;
             }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@PhotoLocatorTest/BenchmarkHelper.cs` around lines 16 - 25, GC.EndNoGCRegion()
is being called unconditionally which can throw if GC.TryStartNoGCRegion(...)
returned false; modify the loop around the benchmark so you capture the boolean
result of GC.TryStartNoGCRegion(1024 * 1024 * 100) into a variable (e.g.,
started), wrap the timed work (Stopwatch sw, the for loop calling action() and
recording sw.ElapsedMilliseconds into iterationTimes) in a try/finally, and only
call GC.EndNoGCRegion() in the finally when started is true so you always end
the region if you started it but never call EndNoGCRegion when
TryStartNoGCRegion failed.

PhotoLocator/BitmapOperations/IIRSmoothOperation.cs (1)

45-46: Redundant unsafe block.

Line 19 already opens an unsafe block, so the nested unsafe on line 45 is superfluous and only adds indentation. Consider removing it for consistency with the horizontal pass above.

♻️ Proposed simplification

-                // Vertical smooth - vectorized over columns when possible
-                unsafe
-                {
-                    int vectorSize = Vector<float>.Count;
-                    int vectorizedSegments = plane.Stride / vectorSize;
-                    var filterSizeV = Vector.Create(filterSize);
-                    var scaleV = Vector.Create(scale);
-                    Parallel.For(0, vectorizedSegments, vi =>
-                    {
-                        ...
-                    });
-                    
-                    // Remaining columns - columns not divisible by vectorSize
-                    Parallel.For(vectorizedSegments * vectorSize, plane.Stride, x =>
-                    {
-                        ...
-                    });
-                }
+                // Vertical smooth - vectorized over columns when possible
+                int vectorSize = Vector<float>.Count;
+                int vectorizedSegments = plane.Stride / vectorSize;
+                var filterSizeV = Vector.Create(filterSize);
+                var scaleV = Vector.Create(scale);
+                Parallel.For(0, vectorizedSegments, vi =>
+                {
+                    ...
+                });
+
+                // Remaining columns - columns not divisible by vectorSize
+                Parallel.For(vectorizedSegments * vectorSize, plane.Stride, x =>
+                {
+                    ...
+                });

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@PhotoLocator/BitmapOperations/IIRSmoothOperation.cs` around lines 45 - 46,
The nested unsafe block inside the IIRSmoothOperation vertical pass is redundant
because an outer unsafe is already in scope; remove the inner "unsafe { ... }"
wrapper and de-indent its contents so the code inside continues to execute under
the existing outer unsafe block (locate the nested unsafe in the
IIRSmoothOperation class and collapse it, mirroring how the horizontal pass is
formatted).